8370041: GenShen: Filter young pointers from thread local SATB buffers when only marking old #27983
Conversation
Otherwise, the filter won't filter out young (possibly garbage) pointers. I don't think this needs to happen on a safepoint. We could probably even do it as part of pausing old GC.
This should be the last and only time it is necessary
|
👋 Welcome back wkemper! A progress list of the required criteria for merging this PR into |
|
@earthling-amzn This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 51 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
openjdk
bot
added
hotspot-gc
|
@earthling-amzn The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
| // be in the collection set. If this happens, the pointer will be preserved, essentially | ||
| // becoming part of the old snapshot. | ||
| // 2. The region is allocated during evacuation of old. This is also not a concern because | ||
| // we haven't yet finished marking old so no mixed evacuations will happen. |
There was a problem hiding this comment.
Might be worth mentioning that there may be some additional analysis and code required when we use the forwarding table to recycle cset regions during evacuation and/or updating. If one of these regions becomes old before the SATB buffers have been flushed, then a young cset pointer that lingers in a SATB buffer will "all of a sudden" look like a valid old pointer and will not be purged from the SATB buffer. When we subsequently scan the "object" referenced by this obsolete pointer, we are likely to find "garbage memory", possibly resulting in a crash.
I am thinking that an initial fix might be to do this flushing at init-update-refs instead of at final update refs, and to not recycle cset regions until evacuation is all done. Is there a different handshake there that we might piggyback on?
There was a problem hiding this comment.
Hmm, that's a good point about recycling regions concurrently. I don't think we can flush before init-update-refs because forwarded pointers still exist and the SATB barrier doesn't try to resolve them. That is to say, "bad" pointers can be created throughout the update-refs phase.
Even in the (hypothetical) scenario with a forwarding table, a region would only become old through an in-place-promotion (in which case, it will never have been recycled) or we will be doing a mixed evacuation. If we are running a mixed evacuation, we will already have finished marking old.
|
I have results from an earlier version of this PR that flushed the buffers during |
|
I found an issue with the verifier that needs to be fixed before this PR is integrated. |
…s in progress This has to happen at least once during the degenerated cycle. Doing it at the start, rather than the end, simplifies the verifier.
|
I've run more tests and confirmed that critical and max Indeed, the experiment looks slightly better in some cases (slightly worse in others). The results also show the expected reduction in safepoint times as we are no longer flushing SATB buffers during The effect is more pronounced on |
There was a problem hiding this comment.
Changes look ok. I found some of the comments confusing -- I have left some remarks at those places, please have a look to see if they can be made clearer.
The improvement in performance looks good. Do we track the number of SATB pointers processed by the old marking (to compare between before and after your changes here)?
| // 1. The region is promoted in place. This is safe because such regions will never | ||
| // be in the collection set. If this happens, the pointer will be preserved, essentially | ||
| // becoming part of the old snapshot. | ||
| // 2. The region is allocated during evacuation of old. This is also not a concern because |
There was a problem hiding this comment.
One related question. In both these cases, I assume the reference will look "marked" because it's above TAMS for the purposes of the old marking?
| // We leave the SATB barrier on for the entirety of the old generation | ||
| // marking phase. In some cases, this can cause a write to a perfectly | ||
| // reachable oop to enqueue a pointer that later becomes garbage (because | ||
| // it points at an object that is later chosen for the collection set). There are |
There was a problem hiding this comment.
// ... In some cases, this can cause a write to a perfectly // reachable oop to enqueue a pointer that later becomes garbage (because // it points at an object that is later chosen for the collection set).
I don't understand this statement. The SATB is supposed to be pointers to objects that we will preserve because they were reachable when the snapshot (marking) was started. Can you elaborate what you mean here? Did you mean that the filtering of the SATB didn't filter a (sometime) young reference which was then processed by the old marking?
| // also cases where the referent of a weak reference ends up in the SATB | ||
| // and is later collected. In these cases the oop in the SATB buffer becomes | ||
| // invalid and the _next_ cycle will crash during its marking phase. To |
There was a problem hiding this comment.
Again I don't understand the concept of an SATB pointer to an object that was later collected? Are we talking about young objects that are subsequently processed by old marking because they weren't filtered out when they should be?
I think that is probably the case here, but it would be good to clean up these comments to avoid this confusion.
3 participants
When GenShen is only marking the old generation, we do not need the SATB mechanism to preserve young pointers. We currently filter these out of the SATB buffers during the final-update-refs and init-mark safepoints. This increases latency and introduces no small amount of complexity. It should be possible to instead filter out these pointers when the SATB buffers are 'compacted' before being 'completed'.
Background
When GenShen is marking the old generation it leaves the SATB barrier enabled. When a young collection interrupts old marking, it creates a situation where a mutator thread could overwrite a field holding a pointer into a collection set region. The SATB barrier will dutifully place this object in the SATB queue. If this pointer makes it into a mark queue, the marking thread will crash. Prior to this change, GenShen filtered out such pointers after the thread local SATB buffers were completed. After this change, such pointers are filtered out before the buffers are completed. This is more inline with the natural way of things.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27983/head:pull/27983$ git checkout pull/27983Update a local copy of the PR:
$ git checkout pull/27983$ git pull https://git.openjdk.org/jdk.git pull/27983/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27983View PR using the GUI difftool:
$ git pr show -t 27983Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27983.diff
Using Webrev
Link to Webrev Comment